Skip to content

Conversation

@dantleech
Copy link
Member

This PR:

  • Allow multiple files to be specified
  • Allow folders to be specified

Its a BC break in that it forces at least one target option to be secified, whereas before there was a required argument for a single CND file.

This PR also keeps in mind support for YAML files instead of CND (I am currently working on a YAML importer + dumper).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was going to say this is jackrabbit specific but then i found http://www.day.com/maven/jcr/2.0/25_Appendix.html#25.2%20Compact%20Node%20Type%20Definition%20Notation which indicates its now somehow part of jcr. did not find it in the chapter on node type management...

@dantleech
Copy link
Member Author

Updated, keeping backwards compatibility, but explicitly marking as depreacated.

@dantleech
Copy link
Member Author

ok, updated to use an argument array instead of options, thanks @dbu

@dantleech
Copy link
Member Author

... and fixed the tests.

@dbu
Copy link
Member

dbu commented May 13, 2014

i think just passing the list as arguments rather than parameter is fine for the new syntax. but we should keep the cnd-file parameter for BC compatibility and only drop it on a new major release.

the tests seem not really fixed.

@dantleech
Copy link
Member Author

ok makes sense, reverted the name of the argument.

The failing test does not seem to be related to this PR.

1) PHPCR\Tests\Util\NodeHelperTest::testBenchmarkOrderBeforeArray

PHP_Invoker_TimeoutException: Execution aborted after 1 second

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could depend on the symfony file component, would be much nicer. but i would say its not worth the additional dependency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah

@dbu
Copy link
Member

dbu commented May 14, 2014

can you please update the doc? then its good to merge imho.

@dantleech
Copy link
Member Author

Updated doc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should say here that it can also be folders and that you can specify several files / folders. (btw, is it a deep recursion or just the folder itself?) folder itself is fine, but we should say it.

@dbu
Copy link
Member

dbu commented May 16, 2014

looking good. could you add a bit to the doc, and then squash your commits?

- Allow multiple files to be specified
- Allow folders to be specified
@dantleech
Copy link
Member Author

Updated + Squashed

dbu added a commit that referenced this pull request May 16, 2014
Refactored node type register command to be more flexible re. loading
@dbu dbu merged commit 7847990 into phpcr:master May 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants